Replace hash groupby internals with HashCSR - #24050
Draft
PointKernel wants to merge 9 commits into
Draft
Conversation
PointKernel
added
libcudf
PointKernel
marked this pull request as draft
September 9, 2026 02:35
|
Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually. Contributors can view more details about this message here. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (24)
💤 Files with no reviewable changes (17)
Included review availability: Your plan provides up to 12 included reviews per hour; 11 remain after this review. 📝 SummarySummary by CodeRabbit
WalkthroughHash groupby now uses HashCSR to create grouped rows and a new single-pass aggregation pipeline. Obsolete cuco-based grouping, shared-memory, global-memory, mapping-index, and sparse-output implementations are removed. ChangesHash groupby migration
Priority: ➖ Normal Estimated code review effort: 4 (Complex) | ~60 minutes Merge Risk: ⚪ Minimal · up to No concrete current-head issue remains from the finalized findings; the PR is mergeable after normal checks. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 5.56% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 36 functions across 4 files. (3 skipped: 3 unsupported.)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
Comment |
Member
Author
|
Groupby benchmarks vs
Slower cases: 4 to 8 groups with 8 aggregations, variance on ~20-row groups with 50% nulls, a single sum at 100M rows, and string keys at 84K groups without nulls. Peak memory is unchanged for distinct keys and 15 to 40% higher when aggregating. groupby_max_cardinality (20M rows, 3 int32 key columns, MAX): 56 faster, 0 slower, 0 same
groupby_max_cardinality, extra axis: 1 to 8 groups: 0 faster, 6 slower, 2 same
groupby_max_cardinality, extra axis: 200K to 2M groups: 4 faster, 2 slower, 0 same
complex_int_keys: 20 faster, 0 slower, 0 same
complex_mixed_keys: 36 faster, 4 slower, 0 same
groupby_max: 35 faster, 1 slower, 0 same
groupby_struct_keys: 18 faster, 0 slower, 0 same
groupby_m2_var_std: 42 faster, 6 slower, 0 same
groupby_m2_var_std, extra axis: 32 to 200 rows per group: 13 faster, 7 slower, 0 same
sum: 6 faster, 2 slower, 0 same
no_requests: 3 faster, 0 slower, 1 same
|
Member
Author
|
Groupby benchmarks vs
Since 67c719a: hot table slots are read through L1; for 2M+ rows with keys up to 32 bytes the table is sized from a sampled distinct-key estimate (full-size rebuild if it falls short); groups averaging under 128 rows are packed several per block via the segmented-reduce size hint, replacing
Slower than groupby_max_cardinality, 20 to 1M groups: 54 faster, 1 slower, 1 same
groupby_max_cardinality, 1 to 16 groups: 18 faster, 2 slower, 0 same
groupby_max_cardinality, 2M to 5M groups: 8 faster, 0 slower, 0 same
complex_int_keys: 20 faster, 0 slower, 0 same
complex_mixed_keys: 33 faster, 7 slower, 0 same
groupby_max: 31 faster, 5 slower, 0 same
groupby_struct_keys: 18 faster, 0 slower, 0 same
groupby_m2_var_std, default axes: 48 faster, 0 slower, 0 same
groupby_m2_var_std, 32 to 1000 rows per group: 56 faster, 0 slower, 0 same
sum: 7 faster, 1 slower, 0 same
no_requests: 3 faster, 0 slower, 1 same
|
Use device-scope table atomics and preserve overflow, nullable, and all-unique grouping invariants. Bound group counters by the input size when the table is larger, and release the table before compaction. Split reduction instantiations across translation units with shared CUB dispatch. Preserve streaming aggregation support and device-view ownership. Add regression coverage for the corrected grouping and streaming behavior.
3 tasks
Member
Author
|
Follow-up to the previous benchmark report, at commit Major changes since
GPU runtime change is the geometric mean of per-case time ratios; negative means faster. The 330 official cases include 16 additional small-input cases beyond the previous report.
The median case is 1.62% slower than the previous PR; the largest slowdown is 0.286 ms. Every measured GPU and synchronized CPU comparison against PR/main satisfies the agreed ≤5% slowdown or <1 ms increase. Selected cases use milliseconds and decimal MB. Peak is requested device memory, listed as main / previous PR / now.
Across 333 official + all-unique cases, peak memory is lower than saved main in 328, equal in 3, and higher by just 500 bytes in each of two keys-only cases. None exceed the previous PR. Eight streaming cases also passed against the saved local reference with identical peaks; no saved upstream streaming baseline is available. Uncached SM120 compilation: grouping 35.3 s, longest measured TU 132.6 s across 17 production and two test objects, reusing earlier timings for unchanged objects. The 150 s/TU limit is not established for multi-architecture builds. Whole-PR self-review and applicable pre-commit hooks passed, along with 119 C++ suites, 25 Python groupby modules, and 21 Compute Sanitizer memcheck tests with zero errors. |
Preserve concurrent streaming aggregation and the cached device-view ownership fix while resolving the overlapping standard-library includes.
This was referenced Sep 12, 2026
bdice
reviewed
Sep 12, 2026
| if (!is_nested(input.type())) { return 1; } | ||
| auto const requested = | ||
| std::max(static_cast<double>(num_rows) + 1, | ||
| std::ceil(static_cast<double>(num_rows) / cudf::detail::CUCO_DESIRED_LOAD_FACTOR)); |
Contributor
There was a problem hiding this comment.
I think this is 0.5 load, but I believe we discussed that higher load factors were still viable with good performance for HashCSR.
| Equal const& d_row_equal, | ||
| Hash const& d_row_hash, | ||
| bool need_grouped_rows, | ||
| cuda::stream_ref stream) |
Contributor
There was a problem hiding this comment.
This should take a cudf::memory_resources for temp MR control.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
This PR replaces the hash groupby internals with a HashCSR build followed by CUB reductions, in the spirit of #23640 for hash join. Rows are grouped by a single probe pass that gives every row a slot and a rank, the occupied slots are compacted into groups, and a fill pass produces the grouped row order. For inputs of two million rows or more with keys up to 32 bytes, the table is sized from a sampled estimate of the number of distinct keys rather than the row count, with a bounded probe that falls back to a slot per row when the estimate is short; this keeps the table cache-resident and roughly halves peak memory on low- and mid-cardinality inputs.
Aggregations are
DeviceSegmentedReducecalls over the grouped order. Groups averaging under 128 rows are packed several per block through the dispatch's segment-size hint (one thread or one 8-lane sub-warp per group), larger groups get a block each, and groups longer than one chunk are reduced in two levels. Nullable aggregations carry the group validity in the accumulator, so a result and its null mask come out of one pass, and the sums behind MEAN, M2, VARIANCE and STD are fused into one reduction.This removes the shared-memory aggregation kernel, the block-local mapping kernel, and the dense and sparse global-memory atomic paths (about 600 lines net). All dispatch is on the host, and because reductions no longer need atomics, decimal128 MIN/MAX and fixed-point SUM_OVERFLOW now take the hash path instead of the sort-based one.
Checklist